add client headers provider func#6362
add client headers provider func#6362kevingentile wants to merge 3 commits intoopen-telemetry:mainfrom
Conversation
| if cfg.headersProvider != nil { | ||
| headers := cfg.headersProvider.Value() | ||
| for k, v := range headers { | ||
| req.Header.Set(k, v) |
There was a problem hiding this comment.
Are we concerned with Key collisions? Should this provider have the highest priority over header control?
There was a problem hiding this comment.
Opted to give the HeadersProvider priority over static Headers. This approach removes an edge case in newRequest.
aaaa075 to
147ba21
Compare
147ba21 to
a8b7a8f
Compare
7d332ad to
0ecbe60
Compare
0ecbe60 to
f14defc
Compare
d95fc5b to
c40e5a3
Compare
|
Please see #5129 for the discussion about this. Also, side note: this PR only does logs, it ignores traces and metrics. |
| type HeadersProviderFunc func() (map[string]string, error) | ||
|
|
||
| // WithHeadersProvider will be called to set the provided headers with each HTTP requests. | ||
| func WithHeadersProvider(providerFunc HeadersProviderFunc) Option { |
There was a problem hiding this comment.
This does not allow to e.g. to refresh token when getting a 401 error.
There was a problem hiding this comment.
The HeadersProvider is executed with every call to newRequest. This should enable implementers to configure a HeadersProviderFunc that can hook into required token lifecycle components for each request. See oauth2/clientcredentials.
There was a problem hiding this comment.
What if the access token is revoked? Do not we want to give the m the possibility to get a new one?
There was a problem hiding this comment.
I think it’s important to point out a constraint in the scope of this effort. These changes are motivated by the need to provide dynamically generated headers with requests, namely an Authorization header whose value may change over time as access tokens are renewed. The implementer is responsible for any scaffolding required to produce the desired headers (or error).
Here is a reference on the client credentials flow that you or others may find useful.
There was a problem hiding this comment.
I think it’s important to point out a constraint in the scope of this effort.
What do you mean? Not gracefully supporting one of the possible oauth scenarios/flows?
I am not sure if it would not be more flexible and easier if we simply add an WithHTTPClient or WithHTTPTransport option (similarly to https://pkg.go.dev/go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc#WithGRPCConn). Then you would be able to use https://pkg.go.dev/golang.org/x/oauth2/clientcredentials#Config.Client. WDYT? I think there was some prior art for it... Probably worth digging into some older issues and PRs
There was a problem hiding this comment.
There was a problem hiding this comment.
My goal here is to provide a way to set dynamic headers, not implement graceful OAuth2 support. Implementers can bring their own. For example:
package main
import (
"context"
"fmt"
"net/url"
"os"
"go.opentelemetry.io/otel"
"go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetrichttp"
"go.opentelemetry.io/otel/sdk/metric"
"golang.org/x/oauth2/clientcredentials"
)
// NOTE: replace in go.mod:
// replace go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetrichttp => github.com/kevingentile/opentelemetry-go/exporters/otlp/otlpmetric/otlpmetrichttp v0.0.0-20250302224340-c40e5a3c2e22
func main() {
ctx := context.Background()
conf := clientcredentials.Config{
ClientID: os.Getenv("CLIENT_ID"),
ClientSecret: os.Getenv("CLIENT_SECRET"),
TokenURL: os.Getenv("TOKEN_URL"),
EndpointParams: url.Values{"audience": {os.Getenv("AUDIENCE")}},
}
exp, err := otlpmetrichttp.New(ctx, otlpmetrichttp.WithHeadersProvider(func() (map[string]string, error) {
token, err := conf.Token(ctx)
if err != nil {
return nil, fmt.Errorf("failed to get token: %w", err)
}
return map[string]string{
"Authorization": "Bearer " + token.AccessToken,
"Some-Dynamic-Header": fmt.Sprint(os.Getpid()),
}, nil
}))
if err != nil {
panic(err)
}
meterProvider := metric.NewMeterProvider(metric.WithReader(metric.NewPeriodicReader(exp)))
defer func() {
if err := meterProvider.Shutdown(ctx); err != nil {
panic(err)
}
}()
otel.SetMeterProvider(meterProvider)
}
There was a problem hiding this comment.
My goal here is to provide a way to set dynamic headers, not implement graceful OAuth2 support.
This PR does not seem to be a proper way to resolve the mentioned issue.
I will try to look into other ways of fixing this issue (see: https://github.com/open-telemetry/opentelemetry-go/pull/6362/files#r1978191352).
@kevingentile, thanks a lot for taking the time to create the PR and discuss it 👍
#4536
Ref: #4536 (comment)